feat(sdk): enforce log record attribute count limit#3571
Conversation
| } | ||
| logger.emit(record); | ||
| provider.force_flush().unwrap(); | ||
| count |
There was a problem hiding this comment.
nit: no need to return the count here.
|
|
||
| // Enforce the configured attribute count limit before handing the | ||
| // record to processors, dropping the attributes added last. | ||
| record.truncate_attributes(provider.max_attributes_per_log() as usize); |
There was a problem hiding this comment.
if the original intend behind enforcing this limit is to protect the sdk from taking up too much memory when a mistaken code adds too much attributes, then we should prevent it from being added in the first place. Truncating afterwards won't help.
Could you try eagerly checking the limit in each add_attribute() call? That'd also make it relatively easy to track the dropped count.
Please do benchmark and see if we regress measurably.
| /// Attributes added beyond this limit are dropped when the record is | ||
| /// emitted. When not set, this is read from the | ||
| /// `OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT` environment variable, which falls | ||
| /// back to `OTEL_ATTRIBUTE_COUNT_LIMIT`, and otherwise defaults to `128`. |
There was a problem hiding this comment.
nit: after this is merged, we should follow up immediately with your other PR so this variable will be consistently applied in spans too.
Wassbdr
force-pushed
the
feat/log-attribute-count-limit
branch
from
a041a17 to
9b5265c
Compare
|
Thanks for the review @cijothomas — addressed all three: Enforce eagerly in Benchmark. Ran
(The machine is a noisy WSL2 box, so the "improved" deltas are noise; the point is nothing regressed. The added cost is a single nit: helper return value / follow-up PR. Removed the unused return from One scope note: this tracks |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3571 +/- ##
=======================================
+ Coverage 83.0% 83.2% +0.1%
=======================================
Files 130 130
Lines 28064 28228 +164
=======================================
+ Hits 23321 23496 +175
+ Misses 4743 4732 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The logs SDK did not enforce any attribute count limit, unlike spans. SdkLoggerProvider now caps the number of attributes retained per log record (default 128). The limit is enforced eagerly in add_attribute so a misbehaving caller cannot grow a record's memory past the limit; rejected attributes are counted and exposed via SdkLogRecord::dropped_attributes_count(). Configurable, in precedence order, via: - LoggerProviderBuilder::with_max_attributes_per_log - OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT - OTEL_ATTRIBUTE_COUNT_LIMIT (general fallback) Part of open-telemetry#3374
Wassbdr
force-pushed
the
feat/log-attribute-count-limit
branch
from
9b5265c to
e4ed69c
Compare
2 participants
Part of #3374. Prerequisite for #3568.
Context
In #3568 I added
OTEL_ATTRIBUTE_COUNT_LIMITfor spans. @cijothomas pointed out that the general variable is meant to apply to logs too, and that the logs SDK does not enforce any attribute limit today — so exposing the general variable before logs support exists would mislead users. This PR adds that missing logs support first.Change
SdkLoggerProvidernow enforces a maximum attribute count per log record (default128), dropping attributes added beyond the limit when the record is emitted. Configurable, in precedence order, via:LoggerProviderBuilder::with_max_attributes_per_log(code-based)OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMITOTEL_ATTRIBUTE_COUNT_LIMIT(general fallback)128Enforcement is done once in
SdkLogger::emit, before the record is handed to processors, using a newGrowableArray::truncate. The limit is resolved at provider build time and stored onLoggerProviderInner.This mirrors the span behavior (
max_attributes_per_span+OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT) and keeps the hot path cheap: a single truncate per emitted record, a no-op when within the limit.Notes / scope
dropped_attributes_countonSdkLogRecordor wire it through OTLP (the proto transform currently sends0). That's a separate, larger change and can follow; happy to do it if preferred.Tests
GrowableArray::truncateunit test (inline region, overflow region, boundary, no-op, zero).OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIToverOTEL_ATTRIBUTE_COUNT_LIMIT; general var as fallback.cargo test -p opentelemetry_sdk --all-features --lib,cargo clippy --all-targets --all-features -- -Dwarnings, andcargo fmt --all -- --checkall pass.